Skip to content

Added secure string support for credentials #1050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 16, 2015
Merged

Added secure string support for credentials #1050

merged 1 commit into from
May 16, 2015

Conversation

rubberduck203
Copy link
Contributor

Issue #1048

I added some extra comments and a helper function in the constants class as well. Obviously I removed my credentials before committing and turned the tests back off, but they passed. You can use this snippet to re-instate them for this change.

        return new SecureUsernamePasswordCredentials() 
        { 
            Username = StringToSecureString("username"), 
            Password = StringToSecureString("swordfish") 
        };

@nulltoken
Copy link
Member

@ckuhn203 Thanks! Looks clean to me.

@ethomson @jamill @Therzok @bording Thoughts?

@rubberduck203
Copy link
Contributor Author

One thing I will say before anyone merges this is that I did consider making the existing credentials and this one share a generic interface, but this was the simplest thing that worked. I figured "Copy paste once: ok. Copy paste twice: stop. Refactor."

@rubberduck203
Copy link
Contributor Author

I might have been quick on the draw there. I'm thinking maybe Username should be a plain string. That's the approach Microsoft took with NetworkCredential. Thoughts?

@nulltoken
Copy link
Member

Thoughts?

Makes sense, indeed. Could you please amend your commit with this change?

Therzok
Therzok reviewed May 16, 2015
View reviewed changes
LibGit2Sharp.Tests/TestHelpers/Constants.cs
var chars = str.ToCharArray();

var secure = new System.Security.SecureString();
for (var i = 0; i < secure.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be chars.Length?

@Therzok
Copy link
Member

Therzok commented May 16, 2015

I'd say keep the Username as a secure string, tbh. The username can be sensitive enough data to brute force the password on some providers. :P

@Therzok
Copy link
Member

Therzok commented May 16, 2015

Actually, nevermind, just saw that NetworkCredential does use a string.

@rubberduck203
Copy link
Contributor Author

Shouldn't this be chars.Length?

Yes. It should have been. I would have thought something would have failed if the wrong password got sent in... Either way, I corrected that.

@rubberduck203
Copy link
Contributor Author

Okay... I don't get it. I've run the tests locally several times. Results seem intermittent. Sometimes everything passes. Sometimes I get failures. Failures are never the same tests and if I simply re-run the failed tests, they pass. Is this a known issue?

@nulltoken
Copy link
Member

I've restarted the build. And it passed. Giving a look at the previous AppVeyor logs, looked like a GitHub transient issue.

@nulltoken
Copy link
Member

🆒

Before we merge this in, could you please squash those commits together into a nice cohesive one? We tend not to merge review commits.

@rubberduck203
Copy link
Contributor Author

Thanks @nulltoken. Much appreciated.

Keep up the good work @ALL. Love the library. Thanks for all your hard work on it (and for helping me to contribute in my own small way)!

nulltoken added a commit that referenced this pull request May 16, 2015
@nulltoken
Merge pull request #1050 from ckuhn203/secureCredentials
ba8b3bf 
Added secure string support for credentials
@nulltoken nulltoken merged commit ba8b3bf into libgit2:vNext May 16, 2015
@nulltoken
Copy link
Member

@ckuhn203 Every contribution is welcome. Yours was very thoughtful! Thanks a lot for it. ✨ ✨

@nulltoken nulltoken added this to the v0.22 milestone May 16, 2015
@rubberduck203 rubberduck203 deleted the secureCredentials branch May 16, 2015 17:52
@rubberduck203 rubberduck203 mentioned this pull request May 16, 2015
Closed
@nulltoken
Copy link
Member

Published as NuGet pre-release package LibGit2Sharp.0.22.0-pre20150516171636

@rubberduck203
Copy link
Contributor Author

👍 thanks again. I'll keep my eye on the easy fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
nuget-pre
Projects
None yet
Milestone
v0.22
Development

Successfully merging this pull request may close these issues.
3 participants
@rubberduck203 @nulltoken @Therzok